-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[docs] Remove unneeded dependencies from examples #9746
[docs] Remove unneeded dependencies from examples #9746
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need some of the dependencies. Otherwise, it's going in the right direction :)
@@ -3,13 +3,10 @@ | |||
"version": "1.0.0", | |||
"private": true, | |||
"dependencies": { | |||
"jss": "latest", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But jss
is listed in material-ui
dependencies, which means that it is installed along with material-ui
and can be imported from node_modules
in any place within application
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a transitive dependency, it's brittle.
"material-ui": "next", | ||
"prop-types": "latest", | ||
"react": "latest", | ||
"react-dom": "latest", | ||
"react-jss": "latest", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
examples/nextjs/package.json
Outdated
@@ -3,13 +3,11 @@ | |||
"version": "1.0.0", | |||
"private": true, | |||
"dependencies": { | |||
"jss": "latest", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
examples/nextjs/package.json
Outdated
"material-ui": "next", | ||
"next": "latest", | ||
"prop-types": "latest", | ||
"react": "latest", | ||
"react-dom": "latest", | ||
"react-jss": "latest" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, hold on. I think that we should add JSS as peer dependency to Material-UI to mitigate the bundle size and class name counter duplication issues. What do you think? |
I think keeping
I believe moving it to Maybe @kof can help us to decide? |
If jss would be something user has to setup, I would go for peerDependency, but I think in case of mui, jss is an internal implementation detail and mui should keep it as far as possible from the user, unless user needs to customize it. |
Here is the issue I'm facing at the office. Our customer-facing components are relying on an up to date version of Material-UI, while our admin backend (admin-on-reset is relying on an older version). Because of I see the following solutions:
|
I have added the transitive dependencies back
Looks like that issue is caused by non-working tree shaking |
I'm not expecting the dependency to be tree shaked. I'm importing v1.0.0-beta.26 |
After all, having JSS as a peer dependency will no help. It will only prevent the bundle size duplication, but it's actually less freedom for our users. For instance, in my case. I'm willing to accept the bundle size duplication in the short term so I can ship my feature. |
One take away: it's important to keep the scope of Material-UI's dependencies as wide as possible. |
v1.0.0-beta.27 has been released, we can update the examples now. I'm merging. @cherniavskii Thank you! |
@oliviertassinari did you consider bundle splitting in your specific case? One bundle for user and other one for admin? And maybe one more bundle with core dependencies? |
@cherniavskii We use Next.js, we have around 30 chunk files. Given how Next.js tell webpack to build the share core bundle, you are right. We would be better off with 2 different Next.js app, one for the customers and one for the admin 👍 . |
@oliviertassinari I should definitely check out Next.js and it's features! |
This PR is continuation of #9735 and is raised by #9732.
Note, that
examples/create-react-app-with-jss
example will not work until beta.27 is released, sincejssPreset
is not exposed in earlier versions.So it's better to not merge it until beta.27 is released.